-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[Web] Fix handling of enabled
prop
#3726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/web/tools/GestureHandlerWebDelegate.ts
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/web/tools/EventManager.ts
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/src/web/handlers/GestureHandler.ts
Show resolved
Hide resolved
} | ||
|
||
public setGestureConfig(config: Config) { | ||
this.resetConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the issue is that we reset the config here, but if the gesture was disabled before, we don't reattach the event listeners. Shouldn't we also try to attach listeners when resetting the config? At first glance, it looks like it could simplify this a lot (especially if it were to be done in a setter).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also try to attach listeners when resetting the config?
But in that case if we have config
that has enabled
set to false
we will first attach and then immediately detach listeners. Is that something that we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reverse that and start with detached listeners which are then attached when config has enabled !== false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of attaching listeners in resetConfig
, we should detach them, and then attach if enabled !== false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the issue is that we reset the config here, but if the gesture was disabled before, we don't reattach the event listeners.
Also, I don't know whether we will have any other logic that depends on enabled
change, but I think that we should say that what the issue really is is that in current setup it is impossible to detect that enabled
has changed to true
*. This may (or may not) cause other troubles in the future.
* If we go through resetConfig
first, with SharedValue
it should be fine.
Description
Right now, if one changes
enabled
property of handlers tofalse
, they can never activate again. In the following example:gesture will never become active, even if
enabled
changes totrue
. Same happens whenenabled
is set totrue
and then changed tofalse
andtrue
again.This happens because we set
enabled
totrue
by default insetGestureConfig
, thus the following check:passes only when
config.enabled
isfalse
.To fix this, I've set
enabled
tonull
by default, and then ifconfig.enabled
is defined, it is assigned given value. Else default will betrue
.Alternative approach would be to follow Android implementation and always react to changes in$\rightarrow$ $\rightarrow$
enabled
even in cases likefalse
false
andtrue
true
.Test plan
Tested on the following code: